Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing tests for multi_ptr aliases for access::address_space::generic #935

Merged
merged 5 commits into from
Oct 25, 2024

Conversation

victor-eds
Copy link
Contributor

Add (decorated_|raw_)generic_ptr aliases definitions.

These aliases were previously referenced in the SYCL specification, but not defined and not tested in the CTS.

Add `(decorated_|raw_)generic_ptr` aliases definitions.

These aliases were previously referenced in the SYCL specification,
but not defined and not tested in the CTS.

Signed-off-by: Victor Perez <victor.perez@codeplay.com>
@victor-eds victor-eds requested a review from a team as a code owner September 9, 2024 10:06
@victor-eds victor-eds changed the title Add missing test for multi_ptr aliases for access::address_space::generic Add missing tests for multi_ptr aliases for access::address_space::generic Sep 9, 2024
@victor-eds
Copy link
Contributor Author

Does not compile as this adds a new alias to the spec.

@psalz
Copy link
Contributor

psalz commented Sep 10, 2024

Does not compile as this adds a new alias to the spec.

Please move the new checks into a compile-time disabled test case!

Signed-off-by: Victor Perez <victor.perez@codeplay.com>
@victor-eds
Copy link
Contributor Author

Please move the new checks into a compile-time disabled test case!

I went with a simple approach as that'd mean having conditionally compiled template functions to avoid using an undefined type and would overcomplicate tests too much to get same degree of coverage.

@victor-eds
Copy link
Contributor Author

Note this same approach is taken in other places like

#if !SYCL_CTS_COMPILING_WITH_HIPSYCL

@psalz
Copy link
Contributor

psalz commented Sep 10, 2024

I went with a simple approach as that'd mean having conditionally compiled template functions to avoid using an undefined type and would overcomplicate tests too much to get same degree of coverage.

It might be simpler but it also hides the fact that an implementation does in fact not fully pass the test case. If you use the DISABLED_FOR_TEST_CASE method, the test will fail at runtime. AdaptiveCpp (formerly hipSYCL) does not strive to achieve conformance with 2020, so the code is still littered with SYCL_CTS_COMPILING_WITH_HIPSYCL. Intel on the other hand made an effort to get rid of all of these before doing their first conformance submission. Adding these back in now means that we have to remember to remove them before the next conformance submission is made.

Edit: I can see that it might be annoying to get it working with the whole typing boilerplate that is going on for those tests. Alternatively you could add an #else branch with an explicit call to FAIL!

@victor-eds
Copy link
Contributor Author

I went with a simple approach as that'd mean having conditionally compiled template functions to avoid using an undefined type and would overcomplicate tests too much to get same degree of coverage.

It might be simpler but it also hides the fact that an implementation does in fact not fully pass the test case. If you use the DISABLED_FOR_TEST_CASE method, the test will fail at runtime. AdaptiveCpp (formerly hipSYCL) does not strive to achieve conformance with 2020, so the code is still littered with SYCL_CTS_COMPILING_WITH_HIPSYCL. Intel on the other hand made an effort to get rid of all of these before doing their first conformance submission. Adding these back in now means that we have to remember to remove them before the next conformance submission is made.

Edit: I can see that it might be annoying to get it working with the whole typing boilerplate that is going on for those tests. Alternatively you could add an #else branch with an explicit call to FAIL!

I defined two additional tests. I still need conditional compilation as they would fail to compile otherwise.

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bader bader requested a review from psalz October 24, 2024 20:38
Copy link
Contributor

@psalz psalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bader bader merged commit eb5eb0c into KhronosGroup:main Oct 25, 2024
8 checks passed
@victor-eds victor-eds deleted the generic-ptr-aliases branch October 25, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants